iio: dac: Add AD5413 support#2786
Conversation
gastmaier
left a comment
There was a problem hiding this comment.
Added the warnings that caused the ci to exit with an error but were not logged
ba5d87d to
1401608
Compare
|
Hi, sorry for the pr failure, I updated the checkout strategy to retry by deepening the fetch until the rebase succeeds. please see CI output, checkpatch is failing for example |
|
Hi @dlech, @mhennerich, @nunojsa, All feedback so far has been addressed in the latest commits. Could you kindly take a look and let me know if anything else is required for approval? Thanks! |
15694b1 to
8174327
Compare
4674362 to
78b10cd
Compare
|
@BruceTsaoADI please properly rebase your patches on top of the target branch. |
6114e4f to
e196cfc
Compare
Replace mutex_lock() and mutex_unlock() calls in rm3100-core.c with the more modern guard(mutex)() family. This will help modernize the driver and bring it up-to-date with modern available macros/functions. While replacing mutex_lock() and mutex_unlock(), the critical sections of rm3100_read_mag() and rm3100_get_samp_freq() have been extended to include negligible operations for cleaner logic. Add new helper-wrapper function rm3100_guarded_regmap_bulk_read() to help keep rm3100_trigger_handler() switch-cases clean while maintaining mutex locking and avoiding re-entrancy risks from potential callbacks. While at it, remove redundant gotos where applicable, and use direct returns instead. In addition, remove regmap variable in rm3100_trigger_handler() as its references have been replaced with variable data. Suggested-by: Jonathan Cameron <[email protected]> Reviewed-by: Andy Shevchenko <[email protected]> Signed-off-by: Maxwell Doose <[email protected]> Signed-off-by: Jonathan Cameron <[email protected]>
Rewrite MCP3422_CHANNEL_MASK, MCP3422_SRATE_MASK, MCP3422_PGA_MASK
and MCP3422_CONT_SAMPLING using GENMASK() and BIT() macros from
bits.h.
The other macros MCP3422_SRATE_{240, 60, 15, 3} were not changed
because they are also used as array indices.
Signed-off-by: Marcelo Machado Lage <[email protected]>
Co-developed-by: Vinicius Lira <[email protected]>
Signed-off-by: Vinicius Lira <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: Jonathan Cameron <[email protected]>
Replace manual bit manipulations with FIELD_GET(), FIELD_PREP() and FIELD_MODIFY() calls. The resulting code is more readable and maintainable, and 6 macros previously defined in the header are not needed anymore. Signed-off-by: Marcelo Machado Lage <[email protected]> Co-developed-by: Vinicius Lira <[email protected]> Signed-off-by: Vinicius Lira <[email protected]> Reviewed-by: Andy Shevchenko <[email protected]> Signed-off-by: Jonathan Cameron <[email protected]>
Use guard(mutex)() for handling mutex lock instead of manually locking and unlocking the mutex. This prevents forgotten locks due to early exits and removes the need of gotos. Signed-off-by: Pedro Barletta Gennari <[email protected]> Reviewed-by: Andy Shevchenko <[email protected]> Signed-off-by: Jonathan Cameron <[email protected]>
Handle errors as early as possible by replacing 'if (!ret)' with the more common form 'if (ret)'. This makes the code easier to read. Signed-off-by: Pedro Barletta Gennari <[email protected]> Reviewed-by: Andy Shevchenko <[email protected]> Signed-off-by: Jonathan Cameron <[email protected]>
patch ci Signed-off-by: CSE CI <[email protected]>
e196cfc to
fdad854
Compare
Document the devicetree bindings for the Analog Devices AD5413, a single-channel precision DAC controlled via SPI. The device supports voltage or current output modes and programmable slew-rate control. Signed-off-by: Bruce Tsao <[email protected]>
Signed-off-by: Bruce Tsao <[email protected]>
d859997 to
f41ce13
Compare
Thanks @nunojsa . I fetched the latest mirror_ci/jic23/iio/testing target branch and rebased the two AD5413 patches on top of fdad854. The PR branch has been force-pushed again. CI is now passing and there are no conflicts with the base branch. |
ae283b1 to
dcb3553
Compare
| #include <linux/property.h> | ||
| #include <linux/spi/spi.h> | ||
| #include <linux/units.h> | ||
| #include <linux/util_macros.h> |
There was a problem hiding this comment.
include paths should be in alphabetical order
|
|
||
| MODULE_AUTHOR("Bruce Tsao <[email protected]>"); | ||
| MODULE_DESCRIPTION("Analog Devices AD5413 DAC"); | ||
| MODULE_LICENSE("GPL v2"); |
There was a problem hiding this comment.
seems like you have some warnings here
| { | ||
| struct ad5413_state *st; | ||
| struct iio_dev *indio_dev; | ||
| int ret, i; |
| return ad5413_spi_write_mask(st, | ||
| AD5413_REG_DAC_CONFIG, | ||
| AD5413_REG_DAC_CONFIG_OUT_EN_MSK, | ||
| FIELD_PREP(AD5413_REG_DAC_CONFIG_OUT_EN_MSK, 1)); |
There was a problem hiding this comment.
still issues with formatting
|
|
||
| *val = ret; | ||
| return IIO_VAL_INT; | ||
| } |
| if (ret < 0) { | ||
| return dev_err_probe(&st->spi->dev, ret, | ||
| "Failed to initiate a calibration memory refresh\n"); | ||
| } |
| /* | ||
| * Poll until the bit clears. ad5413_spi_reg_read() returns a negative | ||
| * errno on failure, so stop polling on error and propagate it. | ||
| */ |
There was a problem hiding this comment.
I would say you don't need to comment what the API does
| u32 cmd; | ||
|
|
||
| cmd = (u32)AD5413_WR_FLAG_MSK(AD5413_REG_TWO_STAGE_READBACK_SELECT) << 24; | ||
| cmd |= (u32)addr << 8; |
| if (ret < 0) | ||
| return ret; | ||
|
|
||
| return (be32_to_cpu(st->d32[2]) >> 8) & 0xFFFF; |
There was a problem hiding this comment.
Oh just noticed this now. Don't mix error codes (returns) with proper read values. It just makes it weird. Just pass in a proper u16 * pointer as a argument and set it:
*val = be32_to_cpu(st->d32[2]) >> 8
Then no need for the mask as we already have an implicit cast to u16
|
@BruceTsaoADI if you rebase again on top of testing so that only your two patches exist, we can trigger a final LLM review on your series before going upstream. |
1719553 to
55ab0e3
Compare
Summary
This PR adds initial support for the Analog Devices AD5413, a 14-bit single-channel DAC capable of voltage and current output.
Key changes
ad5413.cunderdrivers/iio/dac/adi,ad5413.yamlunderDocumentation/devicetree/bindings/iio/dac/Datasheet:
https://www.analog.com/media/en/technical-documentation/data-sheets/ad5413.pdf
PR Description
necessary to understand them. List any dependencies required for this change.
any space), or simply check them after publishing the PR.
description and try to push all related PRs simultaneously.
PR Type
PR Checklist